-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Network Interface Test #605
Conversation
s3_client = s3_client_new(True, self.region, network_interface_names=("eth0", "eth1")) | ||
self.assertIsNotNone(s3_client) | ||
with self.assertRaises(Exception): | ||
s3_client_new(True, self.region, network_interface_names=("eth0", "invalid-network-interface")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we also check the happy case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not easy to test since only a few platforms have a happy case, and even then, it's different for each platform. This test is only verifying that we pass the value to c-s3 correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add a comment here that the functionality was tested in C properly, and here we make sure the binding works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "sanity" in the test name already signifies this intent. I can add a comment if you still think we should include one here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, we have bunch of sanity in the name of the test in aws-c-*, they are meant to simply test it works in the happy case,. eg: https://github.com/awslabs/aws-c-http/blob/main/tests/CMakeLists.txt#L410
I don't think we have a fairly common agreement on "sanity" in the test name means.
It's fine to me. But just mentioning.
*Issues: *
#604
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.